Add smarter type inference for ofType pipe. #1183

rkirov · 2018-07-12T21:13:40Z

It is based on TS2.8 introduced conditional types. Upgrade package.json
to that version.

Also remove the deprecated non-pipable version of ofType so that we
don't have to keep complicated types in two places.

Fixes some tests post removal of non-pipable version.

Original implementation by @mtaran-google.

Fixes #860

coveralls · 2018-07-12T21:15:43Z

Coverage decreased (-0.006%) to 88.394% when pulling 200cb74 on rkirov:oftype_types into 41a0d45 on ngrx:master.

rkirov · 2018-07-12T21:16:32Z

modules/effects/src/actions.ts

 import { filter } from 'rxjs/operators';

+// Removes types from T that are not assignable to U.
+export type Filter<T, U> = T extends U ? T : never;
+
 @Injectable()
 export class Actions<V = Action> extends Observable<V> {


I wonder if I should also remove lift and ctor impl as proposed in #860?

rkirov · 2018-07-13T01:07:51Z

modules/effects/compat/rollup.config.js

@@ -0,0 +1,12 @@
+export default {


I copied those from testing but I have not idea whether they are correct in the context of compat.

timdeschryver · 2018-07-24T12:02:52Z

Should we allow an "infite" number of ofType arguments?

mtaran-google · 2018-07-24T18:29:36Z

That would be ideal, but the type system in TS 2.9 isn't powerful enough to check that many. 3.0 (in RC now) improves on this but I'm not sure if it'll be enough. It'll be interesting to check out when it lands.

timdeschryver · 2018-07-25T06:13:08Z

@mtaran-google I see, thanks.

Just one more thing, I thought the following would give me compile errors but they don't:

// this is OK
ofType<Search>(BookActionTypes.Search),

// I would expect an error here
ofType<Search>('sss'),

// I would expect an error here, because the payload of `SearchComplete` is an array
ofType<Search>(BookActionTypes.SearchComplete),
map(action => action.payload),
  switchMap(query => {
    if (query === '') {
        return empty();
     }

I fiddled with it in the past and came up with this which is giving compile errors.

rkirov · 2018-07-25T21:08:24Z

@timdeschryver That's a good point. Let's recap the desired outcomes:

ofType('x') to just work based in inference
ofType<SomeType>('someActionType') to keep working as is.
ofType<SomeType>('typo') to error if SomeType.type != 'typo' (both of your examples boil down to this).

My current PR has 2) but not 3), and yours I think has 3) but not 2), because you have two free type variables export function ofType<V extends Action, Type extends string> and no defaults. Maybe I am missing something, but I don't think you can have 2) and 3) at the same time.

I think 3) is not as bad as it seems. If we have property 1) eventually, code will be written without any explicit types in <>, thus irrelevant how is the argument vs explicit type annotation resolved. Until then one just has to use the rule-of-thumb "if <> explicitly specified, it wins", i.e. think of it as a type assertion.

rkirov · 2018-07-25T21:53:25Z

However, based on your your example it looks like the T1,T2,..., etc expansions are not necessary. I think the following works the same.

export function ofType<
  V extends Extract<U, { type: T }>,
  T extends string = string,
  U extends Action = Action
>(...t: T[]): OperatorFunction<U, V>;

Also one final thing, my work here is prompted by attempting to turn on --strictFunctionTypes and hitting a lot of ngrx issues. Please do your testing with that flag on. I think any solution that doesn't have at least two parameters U -> V to capture the broader observable input and the narrowed out observable output, will not work with --strictFunctionTypes.

alex-okrushko · 2018-07-25T22:39:27Z

Extract showed up in 2.8, while ngrx is still at 2.7

rkirov · 2018-07-25T23:05:31Z

@alex-okrushko That is the blocker for all this. TS2.8 is required for conditional types, Maksym's original change a custom alias for Extract.

timdeschryver · 2018-07-26T05:30:57Z

@rkirov Yep you're completely right.
The solution I came up with doesn't support ofType<SomeType>('someActionType').

And the snippet you posted has the same outcome.

export function ofType<
  V extends Extract<U, { type: T }>,
  T extends string = string,
  U extends Action = Action
>(...t: T[]): OperatorFunction<U, V>;

rkirov · 2018-07-26T23:45:26Z

Ok, I just discovered one more issue that might be the deal breaker here. A lot of teams do not pass they type argument on their actions observable is it just of type - Actions. Now, if that property is Actions<AllPossibleActions> this change is great.

However, for unqualifed Actions this change (in both proposed forms) is actually a regression. That means if one has private actions: Actions; then this.actions.pipe(ofType('something')) creates Observable<never> instead of at least Observable<Action>.

This is expected because Extract<{type: string}, {type: 'a'}> is never and not {type: 'a'}. In a more general form Extract<string, 'a'> is never though one can expect it to be 'a'. Not sure if TS has plans to change that.

rkirov · 2018-08-01T01:06:25Z

I updated this PR with what is my final proposal for the future types of 'ofType'. Added comments that should capture the upside and downsides of that approach. Hopefully, that would make the type soup a bit more maintainable.

Internally, I appears we have an agreement that this is good and are going to change it with a patch before this PR lands here. Here it appears to be stuck behind angular, bazel, typescript versioning hell, that I can't resolve.

@MikeRyanDev and @brandonroberts please at least verify that there are no major concerns with this proposal, otherwise we would end up forking out internal google users from the mainstream branch here.

MikeRyanDev · 2018-08-01T01:15:17Z

@rkirov This proposed change looks good to me.

brandonroberts · 2018-08-01T15:20:59Z

@rkirov I agree also

timdeschryver · 2018-08-01T18:03:38Z

modules/effects/src/actions.ts

 import { filter } from 'rxjs/operators';

+// Removes types from T that are not assignable to U.
+export type Filter<T, U> = T extends U ? T : never;


Because we're using Extract this can be removed now.

modules/effects/spec/actions.spec.ts

timdeschryver · 2018-08-01T18:10:26Z

modules/effects/src/actions.ts

@@ -19,17 +22,79 @@ export class Actions<V = Action> extends Observable<V> {
    observable.operator = operator;
    return observable;
  }
-
-  ofType<V2 extends V = V>(...allowedTypes: string[]): Actions<V2> {


This may sound stupid, but here it goes anyways:
Since we have the fallback ofType, could we not remove this but mark it as deprecated?
With the next release we can remove it.
I say this because while the compat solution works, the user would have to change the import statement?

I have no opinions on backwards compat (google operates on one-version policy), and will do whatever you guys decide.

I was planning on doing a 6.1 release, so maybe we can deprecate the instance operators before release and then move forward with the removal for the next major release.

alex-okrushko · 2018-08-01T18:48:23Z

package.json

@@ -143,7 +143,7 @@
    "tslib": "1.6.0",
    "tslint": "^5.0.0",
    "tsutils": "2.20.0",
-    "typescript": "~2.7.2",
+    "typescript": "~2.8.4",


I think this means that angular version would need a bump as well:

@angular/compiler-cli@6.0.9 requires typescript@'>=2.7.0 <2.8.0' but 2.8.4 was found instead.

Indeed, at this point this PR is doing too much. Another PR is needed to get to TS 2.8, with all the complications involved around angular, bazel, router(?). I can't do that upgrade, but will rebase this PR once that happens.

rkirov · 2018-08-01T18:58:13Z

Opened #1220 to track the compiler upgrade. This PR is blocked (will respond to review comments though) until that gets resolved.

rkirov · 2018-08-15T05:33:10Z

rebased the PR, will take a look at the CI results tomorrow.

rkirov · 2018-08-15T17:04:29Z

CI is green, please proceed with a regular review. I don't fully understand the compat strategy, mostly copy/pasted from other directories.

timdeschryver · 2018-08-17T13:37:29Z

I think the compat version won't be needed. The store.ofType (and store.select) operators are now marked as deprecated and will be removed the next version.
Before you change anything, wait untill @brandonroberts 's confirmation tho.

brandonroberts · 2018-08-22T04:17:47Z

@rkirov @timdeschryver I think its safe to remove the compat package also. The commit message does need to reflect the breaking change of removing the instance operator.

brandonroberts · 2018-08-22T04:18:21Z

modules/effects/spec/actions.spec.ts

@@ -11,12 +11,20 @@ import { hot, cold } from 'jasmine-marbles';
 import { of } from 'rxjs';

 describe('Actions', function() {
-  let actions$: Actions;
+  let actions$: Actions<AddAcction | SubtractAction>;


AddAcction -> AddAction

alex-okrushko · 2018-09-05T16:09:56Z

@rkirov Hey Rado. Would you be able to take a look at this PR again?

dvitiuk-opensource · 2018-10-01T13:00:05Z

Any update on this? I can finalize this PR, if you don't mind.

For someone who wants to try this feature in your project today, you can add this to your codebase:

declare module '@ngrx/effects' {
  export function ofType<
    V extends Extract<U, { type: T }>, 
    T extends string = string, 
    U extends Action = Action
  >(...actions: T[]): OperatorFunction<U, V>;
}

timdeschryver · 2018-10-02T14:07:10Z

If you want I can finish this one up @rkirov but you'll have to give maintainers write access to your branch.

rkirov · 2018-10-02T18:17:44Z

I will rebase it today. Sorry for the delay.

Btw, we have been running this with change internally in google for some time and I haven't heard any complaints.

@mtaran-google

It is based on TS2.8 introduced conditional types. Upgrade package.json to that version. Also remove the deprecated non-pipable version of `ofType` so that we don't have to keep complicated types in two places. Fixes some tests post removal of non-pipable version. Original implementation by @mtaran-google. BREAKING CHANGES: Removes .ofType method on Actions. Instead use the provided 'ofType' rxjs operator. BEFORE: ``` this.actions.ofType('INCREMENT') ``` AFTER: ``` import { ofType } from '@ngrx/store'; ... this.action.pipe(ofType('INCREMENT')) ```

rkirov · 2018-10-03T19:46:57Z

Rebased, removed compat bits and updated commit message with 'BREAKING CHANGE'. Please take another look.

timdeschryver

👍

brandonroberts · 2018-10-16T02:27:51Z

Thanks @rkirov!

rkirov force-pushed the oftype_types branch 4 times, most recently from 35b3ab4 to d04cae3 Compare July 13, 2018 00:58

rkirov commented Jul 13, 2018

View reviewed changes

rkirov force-pushed the oftype_types branch from d04cae3 to b0b52d9 Compare July 14, 2018 01:29

rkirov force-pushed the oftype_types branch from b0b52d9 to 883cb46 Compare July 23, 2018 18:41

rkirov force-pushed the oftype_types branch from 883cb46 to 7b608fb Compare August 1, 2018 00:56

timdeschryver reviewed Aug 1, 2018

View reviewed changes

alex-okrushko reviewed Aug 1, 2018

View reviewed changes

rkirov force-pushed the oftype_types branch from 7b608fb to 67100dd Compare August 1, 2018 18:54

rkirov mentioned this pull request Aug 1, 2018

upgrade to TS 2.8+ #1220

Closed

brandonroberts mentioned this pull request Aug 11, 2018

Update documentation with new usage of ofType operator #1239

Closed

rkirov force-pushed the oftype_types branch from 67100dd to 637151a Compare August 15, 2018 05:43

brandonroberts reviewed Aug 22, 2018

View reviewed changes

jayphelps mentioned this pull request Sep 6, 2018

Automatically narrow Action type in the ofType operator redux-observable/redux-observable#459

Closed

2 tasks

rkirov force-pushed the oftype_types branch from 637151a to baeebfc Compare October 3, 2018 19:45

rkirov force-pushed the oftype_types branch from baeebfc to 200cb74 Compare October 3, 2018 19:45

timdeschryver approved these changes Oct 3, 2018

View reviewed changes

brandonroberts merged commit 8d56a6f into ngrx:master Oct 16, 2018

Add smarter type inference for ofType pipe. #1183

Add smarter type inference for ofType pipe. #1183

Conversation

rkirov commented Jul 12, 2018

coveralls commented Jul 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timdeschryver commented Jul 24, 2018 • edited Loading

mtaran-google commented Jul 24, 2018

timdeschryver commented Jul 25, 2018 • edited Loading

rkirov commented Jul 25, 2018 • edited Loading

rkirov commented Jul 25, 2018

alex-okrushko commented Jul 25, 2018

rkirov commented Jul 25, 2018

timdeschryver commented Jul 26, 2018

rkirov commented Jul 26, 2018 • edited Loading

rkirov commented Aug 1, 2018

MikeRyanDev commented Aug 1, 2018

brandonroberts commented Aug 1, 2018

timdeschryver Aug 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkirov commented Aug 1, 2018

rkirov commented Aug 15, 2018

rkirov commented Aug 15, 2018

timdeschryver commented Aug 17, 2018

brandonroberts commented Aug 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex-okrushko commented Sep 5, 2018

dvitiuk-opensource commented Oct 1, 2018

timdeschryver commented Oct 2, 2018

rkirov commented Oct 2, 2018

rkirov commented Oct 3, 2018

timdeschryver left a comment

Choose a reason for hiding this comment

brandonroberts commented Oct 16, 2018

coveralls commented Jul 12, 2018 •

edited

Loading

timdeschryver commented Jul 24, 2018 •

edited

Loading

timdeschryver commented Jul 25, 2018 •

edited

Loading

rkirov commented Jul 25, 2018 •

edited

Loading

rkirov commented Jul 26, 2018 •

edited

Loading

timdeschryver Aug 1, 2018 •

edited

Loading